Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mb_convert_encoding does not support windows-1250. #1037 #8327

Conversation

Astinus-Eberhard
Copy link

No description provided.

@welcome
Copy link

welcome bot commented Apr 3, 2023

Thanks for opening your first pull request in this repository! ✌️

try {
$data = mb_convert_encoding($data, 'UTF-8', $charset);
} catch (\Throwable $ex) {
$data = iconv($charset, 'UTF-8//TRANSLIT', $data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is translit generally available or does it depend on the compilation options of iconv? we had UTF8//IGNORE in the past and it was troublesome

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea, but used a lot in so many projects & environments and I haven't spotted a single issue. I found both //TRANSLIT and //IGNORE options everywhere I read anything about Iconv. Honestly I've never even think this can failed (at least with clear from and to encodings and without autodetections).

There was some problems in some Iconv implementations, for example on some docker alphine images, but IMHO nothing so unusual & there was some fixes for it (mainly updates binaries from edges).

This PR is as a suggestion, if someone has a better experience with character encoding then please review and replace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3908 was the older thread about //IGNORE issues.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I see no risk here, cause this change does even matter only if there was an exception thrown, which is currently not handled properly, preventing some users from reading emails with corrupted encodings, which IMHO is more annoying than potential risk of misleading characters replacement. If under any environment this will fail, then again, now it is failing anyway too, so nothing change really.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The risk is that there are possibly environments that currently work and will break with this change.

Could you check if Alpine's iconv handles //TRANSLIT without any issues?

#3908 (comment) for the specific issues we had earlier.

If translit works with all iconv implementations I'm okay with this change 👍

Thanks

@ririko5834
Copy link

When it willl be merged

@Astinus-Eberhard Astinus-Eberhard force-pushed the bug/1037/iconv_as_fallback_for_mb_convert_encoding branch from 46947cb to a47ca70 Compare August 2, 2023 22:37
@Astinus-Eberhard Astinus-Eberhard force-pushed the bug/1037/iconv_as_fallback_for_mb_convert_encoding branch from a47ca70 to 1230144 Compare August 2, 2023 22:37
@ririko5834
Copy link

Does anyone think that this PR can fix my issue anonaddy/anonaddy#485

@ChristophWurst ChristophWurst mentioned this pull request Nov 29, 2023
24 tasks
@ChristophWurst
Copy link
Member

Closing due to lack of clarity regarding the support of //TRANSLIT across platforms #8327 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants